Skip to content

Assignment 2#2

Open
mandana-g wants to merge 9 commits into
mainfrom
assignment-2
Open

Assignment 2#2
mandana-g wants to merge 9 commits into
mainfrom
assignment-2

Conversation

@mandana-g
Copy link
Copy Markdown
Owner

What changes are you trying to make? (e.g. Adding or removing code, refactoring existing code, adding reports)

adding codes

What did you learn from the changes you have made?

practice working with "numpy" and working with files.

Was there another approach you were thinking about making? If so, what approach(es) were you thinking of?

I tried to read inflammation files instead of referring to their list. This helps to be more maintainable.

Were there any challenges? If so, what issue(s) did you face? How did you overcome it?

there was a confusion between using arrays in python and "numpy" and I need to refer to references.

How were these changes tested?

by running the code for existing and also modified files so that they cover all scenarios.

A reference to a related issue in your repository (if applicable)

Checklist

  • I can confirm that my changes are working as intended

Copy link
Copy Markdown

@anjali-deshpande-hub anjali-deshpande-hub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question 1:

  1. Move the declaration of all_paths variable to the very first code-block which answers question 1. (Note: you have the declaration down below in the code. )
  2. Pass all_paths[0] to open function instead of hard-coded file name (as given in the assignment). That way you don't need the check if i==1
  3. Instead of range(60), use len(contents). You don't need to hard-code 60 in the code. That way when number of patients increase, you don't have to modify your code.

Question 2:
In code blocks like the one shown below, you don't need the for loop and hard-coded 60.


for i in range(60):
                summary_values[i,0] = np.mean(data[i])

Simply use:
summary_values = np.mean(data, axis=ax);

Please make the above suggested changes.

@mandana-g
Copy link
Copy Markdown
Owner Author

mandana-g commented Dec 11, 2024

Thanks for the comments Anjali. I made the changes regarding using len(contents) and np.mean(data, axis=ax) and updated the PR . For #1 and #2 in Question1, I actually tried to code a bit more comprehensive solution for reading all files and print only the first one as requested. That is why I called all_paths[] down below where actually it was required. Please let me now If it is making any inefficiencies. Thank you.

@anjali-deshpande-hub
Copy link
Copy Markdown

The above mentioned issues have all been fixed. Well done!

There is one thing I overlooked in my first review. assignment_1.ipynb was also changed in assignment_2 branch and therefore shows as changed in this PR. This PR (and the branch) should only have assignment_2.ipynb changes. You can fix this using following steps here: https://uoft-dsi-certificates.slack.com/archives/C080M1JFJ8G/p1733888574840979

@mandana-g
Copy link
Copy Markdown
Owner Author

Assignment_1 reverted to main branch.

Copy link
Copy Markdown

@anjali-deshpande-hub anjali-deshpande-hub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although the assignment_1.ipynb file shows as changed in the PR (https://github.com/mandana-g/python/pull/2/files), the change is only in the comment i metadata, which is fine.
Well done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants